Extend expression fuzzer test to support decimal #9149

rui-mo · 2024-03-19T07:48:06Z

ArgumentTypeFuzzer could be used to generate argument types when constructing a
decimal expression in the fuzzer test. However, given a result type, it is
unable to produce argument types that satisfy the necessary constraints. To
address this limitation, argument type generators for Presto and Spark decimal
functions have been added. In this PR, ExpressionFuzzer takes a map from
function name to an instance of the argument generator. Custom generators
provided by Presto and Spark are used in the expression fuzzer test to generate
argument types. Experimental fuzzer tests with decimal type enabled will be
added in a follow-up PR.

#1968

netlify · 2024-03-19T07:48:41Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`70cf156`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66d8f878d7dc8100087a45c3

rui-mo · 2024-03-19T08:09:40Z

Hi @mbasmanova, I drafted this PR to support decimal in expression fuzzer test. Tested decimal_add and decimal_between locally for 10 minutes each. Below are some next-steps I can think of.

Verify other decimal functions as well (agg functions are tested in AggregationFuzzer, so they will not be covered in this PR).
Fix CI issues and enable velox_fuzzer_enable_decimal_type in CI.
Test decimal function registered as simple function when the relating PR is merged.

Besides that, do you have more items in mind which I should take care of to move forward? Thank you.

mbasmanova

@rui-mo Rui, thank you for extending Fuzzer to cover decimal types. Would you update the PR description to provide some details about the design of this extension? Are there any limitations?

@kagamiori @kgpai Krishna, Wei, would you help review this PR?

CC: @majetideepak @aditi-pandit

rui-mo · 2024-03-26T13:30:29Z

This PR is updated and ready for review. Thanks.

velox/functions/prestosql/DecimalFunctions.cpp

velox/functions/sparksql/MakeTimestamp.cpp

velox/expression/tests/utils/ArgumentTypeFuzzer.cpp

velox/expression/ConstantExpr.cpp

velox/expression/tests/ExpressionFuzzer.cpp

majetideepak

workflow changes look good!

rui-mo · 2024-07-18T00:41:03Z

@majetideepak @assignUser Thanks for your review. The workflow fails with below error as the main branch is used for testing. It should be fine after this PR is landed.

ERROR: unknown command line flag 'velox_fuzzer_enable_decimal_type'

majetideepak · 2024-07-18T14:53:10Z

@rui-mo A safer option is to move the dependent workflow changes to a new PR. It is always safer to avoid landing changes when CI is failing.

rui-mo · 2024-07-19T01:58:23Z

A safer option is to move the dependent workflow changes to a new PR. It is always safer to avoid landing changes when CI is failing.

@majetideepak Thanks for the suggestion. Removed workflow changes.

majetideepak · 2024-07-19T13:33:14Z

@rui-mo thanks! This PR is already approved and tagged for merging. Meta will merge this.

rui-mo · 2024-08-07T13:51:23Z

@xiaoxmeng Would you help import and merge this PR? Thanks.

rui-mo · 2024-08-14T02:56:36Z

Hi @xiaoxmeng, does this PR also require some internal work from Meta? Wondering if there is any chance to land this change first. Thanks.

facebook-github-bot · 2024-09-04T17:37:08Z

@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

assignUser

The last commit in this PR was almost a month ago. Can we rebase it before merging please.

rui-mo · 2024-09-05T03:59:55Z

@assignUser @gggrace14 Rebased this PR, and the macOS build failure looks relevant to #10886. Would you take a another look? Thank you.

assignUser

Thanks!

pedroerp · 2024-09-06T20:38:03Z

We are seeing this problem where PRs that were created before we enabled the notification system do not trigger a ready-to-merge notification. Apologies for the delay on this one.

facebook-github-bot · 2024-09-07T08:19:35Z

@gggrace14 merged this pull request in 030e439.

conbench-facebook · 2024-09-07T08:45:56Z

Conbench analyzed the 1 benchmark run on commit 030e439f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

rui-mo · 2024-09-08T02:50:05Z

@gggrace14 @pedroerp Thanks for noticing and helping land this PR!

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 19, 2024

mbasmanova requested review from kagamiori and kgpai March 19, 2024 12:14

mbasmanova reviewed Mar 19, 2024

View reviewed changes

rui-mo force-pushed the wip_dec_fuzzer branch 4 times, most recently from 3714ccc to 3e340cd Compare March 26, 2024 10:00

rui-mo changed the title ~~Support decimal in expression fuzzer test~~ Extend expression fuzzer test to support decimal Mar 26, 2024

rui-mo marked this pull request as ready for review March 26, 2024 13:27

rui-mo requested a review from mbasmanova March 26, 2024 13:30

mbasmanova reviewed Mar 26, 2024

View reviewed changes

velox/functions/prestosql/DecimalFunctions.cpp Outdated Show resolved Hide resolved

mbasmanova reviewed Mar 26, 2024

View reviewed changes

velox/functions/sparksql/MakeTimestamp.cpp Outdated Show resolved Hide resolved

mbasmanova reviewed Mar 26, 2024

View reviewed changes

velox/expression/tests/utils/ArgumentTypeFuzzer.cpp Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_dec_fuzzer branch from 3e340cd to 353d82d Compare March 28, 2024 04:36

rui-mo commented Mar 28, 2024

View reviewed changes

velox/expression/ConstantExpr.cpp Outdated Show resolved Hide resolved

rui-mo requested a review from mbasmanova March 28, 2024 07:38

mbasmanova reviewed Mar 28, 2024

View reviewed changes

velox/expression/tests/ExpressionFuzzer.cpp Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_dec_fuzzer branch 5 times, most recently from 6ee7848 to 70e0b27 Compare April 1, 2024 00:56

rui-mo marked this pull request as draft April 1, 2024 01:21

rui-mo force-pushed the wip_dec_fuzzer branch 3 times, most recently from 8251b61 to 569a1c8 Compare April 1, 2024 05:57

majetideepak approved these changes Jul 17, 2024

View reviewed changes

rui-mo force-pushed the wip_dec_fuzzer branch 2 times, most recently from ea7a940 to 325aef2 Compare July 19, 2024 01:57

assignUser added ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall and removed ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall labels Jul 19, 2024

rui-mo force-pushed the wip_dec_fuzzer branch from 325aef2 to a7dca04 Compare July 25, 2024 07:17

rui-mo force-pushed the wip_dec_fuzzer branch from a7dca04 to a397f67 Compare August 7, 2024 08:15

rui-mo force-pushed the wip_dec_fuzzer branch from a397f67 to 451cea3 Compare August 14, 2024 02:56

assignUser requested changes Sep 4, 2024

View reviewed changes

assignUser removed the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Sep 4, 2024

Add support for DECIMAL types to Expression Fuzzer

70cf156

rui-mo force-pushed the wip_dec_fuzzer branch from 451cea3 to 70cf156 Compare September 5, 2024 00:16

assignUser approved these changes Sep 5, 2024

View reviewed changes

assignUser added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Sep 5, 2024

rui-mo mentioned this pull request Sep 6, 2024

Fuzzer generates UTF8 chars are used to convert to integer #10880

Closed

facebook-github-bot closed this in 030e439 Sep 7, 2024

facebook-github-bot added the Merged label Sep 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend expression fuzzer test to support decimal #9149

Extend expression fuzzer test to support decimal #9149

rui-mo commented Mar 19, 2024 •

edited

Loading

netlify bot commented Mar 19, 2024 •

edited

Loading

rui-mo commented Mar 19, 2024 •

edited

Loading

mbasmanova left a comment

rui-mo commented Mar 26, 2024

majetideepak left a comment

rui-mo commented Jul 18, 2024

majetideepak commented Jul 18, 2024

rui-mo commented Jul 19, 2024

majetideepak commented Jul 19, 2024

rui-mo commented Aug 7, 2024

rui-mo commented Aug 14, 2024 •

edited

Loading

facebook-github-bot commented Sep 4, 2024

assignUser left a comment

rui-mo commented Sep 5, 2024

assignUser left a comment

pedroerp commented Sep 6, 2024

facebook-github-bot commented Sep 7, 2024

conbench-facebook bot commented Sep 7, 2024

rui-mo commented Sep 8, 2024

Extend expression fuzzer test to support decimal #9149

Extend expression fuzzer test to support decimal #9149

Conversation

rui-mo commented Mar 19, 2024 • edited Loading

netlify bot commented Mar 19, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

rui-mo commented Mar 19, 2024 • edited Loading

mbasmanova left a comment

Choose a reason for hiding this comment

rui-mo commented Mar 26, 2024

majetideepak left a comment

Choose a reason for hiding this comment

rui-mo commented Jul 18, 2024

majetideepak commented Jul 18, 2024

rui-mo commented Jul 19, 2024

majetideepak commented Jul 19, 2024

rui-mo commented Aug 7, 2024

rui-mo commented Aug 14, 2024 • edited Loading

facebook-github-bot commented Sep 4, 2024

assignUser left a comment

Choose a reason for hiding this comment

rui-mo commented Sep 5, 2024

assignUser left a comment

Choose a reason for hiding this comment

pedroerp commented Sep 6, 2024

facebook-github-bot commented Sep 7, 2024

conbench-facebook bot commented Sep 7, 2024

rui-mo commented Sep 8, 2024

rui-mo commented Mar 19, 2024 •

edited

Loading

netlify bot commented Mar 19, 2024 •

edited

Loading

rui-mo commented Mar 19, 2024 •

edited

Loading

rui-mo commented Aug 14, 2024 •

edited

Loading